Skip to content

fix: address low-priority review items from Bun→Node migration#991

Merged
BYK merged 1 commit into
mainfrom
byk/cleanup-bun-node-migration
May 20, 2026
Merged

fix: address low-priority review items from Bun→Node migration#991
BYK merged 1 commit into
mainfrom
byk/cleanup-bun-node-migration

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented May 20, 2026

Context

Addresses all 5 cleanup items from #990 — low-priority review items deferred during the Bun → Node.js migration.

Changes

1. Remove dead SQLite polyfill code

  • Removed NodeDatabasePolyfill, NodeStatementPolyfill, and bunSqlitePolyfill from script/node-polyfills.ts
  • Removed bunSqlitePlugin from script/bundle.ts
  • These were replaced by src/lib/db/sqlite.ts which handles both runtimes directly

2. Fix which polyfill — timeout + command -v

  • Changed from which binary to command -v (POSIX shell builtin) on Unix — works with restricted PATH
  • Added timeout: 5000 to execSync call to prevent indefinite blocking
  • Windows still uses where (no command -v equivalent)

3. Add stream cleanup to bspatch BufferedStreamReader

  • Added cancel() method to BufferedStreamReader that calls reader.cancel() and reader.releaseLock()
  • Updated applyPatch finally block to clean up diffReader and extraReader on error paths
  • Prevents leaking native DecompressionStream state

4. Fix scanner TOCTOU

  • Replaced separate readFile() then stat() calls with a single fs.promises.open() file handle
  • Content and mtime are now read from the same handle, eliminating the race window
  • Kept isRegularFile() pre-check to avoid blocking open() on FIFOs (1Password guard)

5. Add CI check for patched dependency versions

  • New script/check-patches.ts — compares patch file target versions against installed versions
  • Added check:patches script to package.json
  • Added to CI lint job in .github/workflows/ci.yml
  • Catches silent patch skips when a dependency is bumped past the patch version

Also updated .lore.md to fix a stale reference to the non-existent src/lib/which.ts.

Closes #990

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026 •
edited
Loading

PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://cli.sentry.dev/_preview/pr-991/

Built to branch gh-pages at 2026-05-20 20:15 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

cursor[bot]
cursor Bot reviewed May 20, 2026
View reviewed changes
Comment thread src/lib/dsn/scanner.ts Outdated
sentry[bot]
sentry Bot reviewed May 20, 2026
View reviewed changes
Comment thread src/lib/dsn/scanner.ts Outdated
@BYK BYK force-pushed the byk/cleanup-bun-node-migration branch 3 times, most recently from 4e26519 to cfac9d3 Compare May 20, 2026 19:14
sentry-warden[bot]
sentry-warden Bot reviewed May 20, 2026
View reviewed changes
Copy link
Copy Markdown

@sentry-warden sentry-warden Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell injection in Bun.which() polyfill via unquoted command interpolation

The Bun.which() polyfill builds a shell string by directly interpolating command — command -v ${command} on Unix and where ${command} on Windows — and passes it to execSync, which invokes /bin/sh -c (or cmd.exe). A command value containing shell metacharacters (;, $(...), backticks) would execute arbitrary code. The safe pattern is already in src/lib/which.ts: use execFileSync("/bin/sh", ["-c", 'command -v "$1"', "--", command], ...) so command is never interpolated into the shell string.

Evidence
  • execSync(cmd, ...) at script/node-polyfills.ts:93 passes a string, which Node.js runs via /bin/sh -c on Unix, making it vulnerable to metacharacter injection.
  • cmd is built on line 85 as `command -v ${command}` with no sanitization of command.
  • src/lib/which.ts:51-57 shows the correct fix already exists in the codebase: execFileSync("/bin/sh", ["-c", 'command -v "$1"', "--", command]) passes command as a positional arg, never interpolated.
  • globalThis.Bun = BunPolyfill (end of polyfill file) installs this as the global Bun.which, reachable by any future caller in the bundled runtime.
  • Tests call Bun.which("node") / Bun.which("realpath") with hardcoded strings only, so no current caller exercises a malicious payload.

Identified by Warden find-bugs

Comment thread src/lib/bspatch.ts
@BYK BYK force-pushed the byk/cleanup-bun-node-migration branch 2 times, most recently from 197f251 to c0d1fea Compare May 20, 2026 19:57
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026 •
edited
Loading

Codecov Results 📊

✅ 7014 passed | Total: 7014 | Pass Rate: 100% | Execution Time: 1ms

📊 Comparison with Base Branch

Metric Change
Total Tests —
Passed Tests —
Failed Tests —
Skipped Tests —

✨ No test changes detected

All tests are passing successfully.

✅ Patch coverage is 85.71%. Project has 14253 uncovered lines.
❌ Project coverage is 77.09%. Comparing base (base) to head (head).

Files with missing lines (1)
File Patch % Lines
src/lib/bspatch.ts 78.95% ⚠️ 8 Missing
Coverage diff 
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
- Coverage    77.11%    77.09%    -0.02%
==========================================
  Files          322       322         —
  Lines        62167     62211       +44
  Branches         0         0         —
==========================================
+ Hits         47938     47958       +20
- Misses       14229     14253       +24
- Partials         0         0         —

Generated by Codecov Action

cursor[bot]
cursor Bot reviewed May 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c0d1fea. Configure here.

Comment thread src/lib/bspatch.ts
@BYK BYK force-pushed the byk/cleanup-bun-node-migration branch from c0d1fea to 9d5d998 Compare May 20, 2026 20:05
@BYK
fix: address low-priority review items from Bun→Node migration (#990)
2e73b04 
1. Remove dead SQLite polyfill code (NodeDatabasePolyfill + bunSqlitePlugin)
   — replaced by src/lib/db/sqlite.ts

2. Fix which polyfill: use 'command -v' (POSIX builtin) instead of 'which'
   binary, add 5s timeout to prevent indefinite blocking

3. Add stream cleanup to bspatch BufferedStreamReader: cancel() method +
   cleanup in applyPatch finally block

4. Fix scanner TOCTOU: use single fs.promises.open() file handle for
   atomic read + stat (content and mtime from same handle)

5. Add CI check (check:patches) to detect when pnpm patchedDependencies
   target versions diverge from installed versions

Closes #990
@BYK BYK force-pushed the byk/cleanup-bun-node-migration branch from 9d5d998 to 2e73b04 Compare May 20, 2026 20:14
@BYK BYK merged commit 426e9e1 into main May 20, 2026
30 checks passed
@BYK BYK deleted the byk/cleanup-bun-node-migration branch May 20, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sentry sentry[bot] sentry[bot] left review comments

@cursor cursor[bot] cursor[bot] left review comments

@sentry-warden sentry-warden[bot] sentry-warden[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cleanup: address low-priority review items from Bun→Node migration

1 participant

@BYK